This repository has been archived by the owner on Nov 17, 2023. It is now read-only.
[Numpy]flip #15819
Merged
[Numpy]flip #15819
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
haojin2
reviewed
Aug 15, 2019
src/operator/numpy/np_flip_op.cc
Outdated
| @@ -0,0 +1,67 @@ | |||
| /* | |||
There was a problem hiding this comment.
Move the code to the existing np_matrix_op* files.
haojin2
reviewed
Aug 15, 2019
| rtol=1e-3 | ||
| atol=1e-5 | ||
| for shape in shapes1: | ||
| for axis in [None]: |
There was a problem hiding this comment.
If you just have 1 thing to test then do not have a for-loop, just do
for shape in shapes1:
axis = None
# your test codeThere was a problem hiding this comment.
Fixed, thank you.
haojin2
reviewed
Aug 15, 2019
| class TestFlip(HybridBlock): | ||
| def __init__(self, axis): | ||
| super(TestFlip, self).__init__() | ||
| # Necessary initialization |
There was a problem hiding this comment.
Fixed, thank you.
haojin2
reviewed
Aug 15, 2019
| for oneType in types: | ||
| if oneType is 'float16': | ||
| rtol=1e-2 | ||
| atol=1e-2 |
There was a problem hiding this comment.
why do you need higher tolerance for float16 type? flip is a kind of copy, there should be no numerical instability issues.
There was a problem hiding this comment.
Fixed, thank you.
haojin2
reviewed
Aug 15, 2019
|
|
||
| @with_seed() | ||
| @use_np | ||
| def test_np_flip(): |
There was a problem hiding this comment.
You do not need to add the same test once again in test_operator_gpu.py there's an import up above already. Delete this function.
There was a problem hiding this comment.
Done, thank you.
haojin2
reviewed
Sep 4, 2019
tingying2020
force-pushed
the
ms_flip
branch
3 times, most recently
from
386d6e8
to
554517b
Compare
Laurawly
reviewed
Sep 11, 2019
| return F.np.flip(x, self.axis) | ||
|
|
||
| shapes1 = [(1, 2, 3), (1, 0), (3, 0, 2), (), (1,)] | ||
| shapes2 = [(2, 2, 3, 3), (1, 1, 2, 4)] |
There was a problem hiding this comment.
Can we merge the test cases of shapes1 and shapes2 into one for loop?
haojin2
reviewed
Sep 12, 2019
| def hybrid_forward(self, F, x): | ||
| return F.np.flip(x, self.axis) | ||
|
|
||
| shapes1 = [(1, 2, 3), (1, 0), (3, 0, 2), (), (1,)] |
There was a problem hiding this comment.
shapes = [(1, 2, 3), (1, 0), ()]
haojin2
reviewed
Sep 12, 2019
| mx_out = np.flip(x, axis) | ||
| np_out = _np.flip(x.asnumpy(), axis) | ||
| assert_almost_equal(mx_out.asnumpy(), np_out, rtol=rtol, atol=atol) | ||
| for shape in shapes2: |
There was a problem hiding this comment.
then you can get rid of this loop.
haojin2
reviewed
Sep 12, 2019
| if (inputs[0].shape_.ndim() == 0) { | ||
| mshadow::Stream<xpu> *s = ctx.get_stream<xpu>(); | ||
| MSHADOW_TYPE_SWITCH(outputs[0].type_flag_, DType, { | ||
| mxnet_op::Kernel<flip0dim_shared_kernel, xpu>::Launch(s, inputs[0].Size(), |
There was a problem hiding this comment.
Please directly call copy function
haojin2
reviewed
Sep 12, 2019
| } | ||
| }; | ||
|
|
||
| struct flip0dim_shared_kernel { |
tingying2020
force-pushed
the
ms_flip
branch
4 times, most recently
from
8479421
to
b2755af
Compare
Laurawly
approved these changes
Sep 17, 2019
tingying2020
force-pushed
the
ms_flip
branch
from
b2755af
to
708276d
Compare
* Implement flip * fix some bug and add gpu test * register param and edit test * add testcase for backward * remove print * optimize 0-dim and 0-shape * adjust format and add doc in _symbol.py * fix bug in symbol * add flip in __all__ * fix format error * import ndarray * move flip implementation to np_matrix_op and remove test in gpu * delate redundant blank line * fix error in review * remove **kwargs and change copy * fix error in review
drivanov
pushed a commit
to drivanov/incubator-mxnet
that referenced
this pull request
Sep 26, 2019
* Numpy flip operator * Implement flip * fix some bug and add gpu test * register param and edit test * add testcase for backward * remove print * optimize 0-dim and 0-shape * adjust format and add doc in _symbol.py * fix bug in symbol * add flip in __all__ * fix format error * import ndarray * move flip implementation to np_matrix_op and remove test in gpu * delate redundant blank line * fix error in review * remove **kwargs and change copy * fix error in review * Fix import * Fix lint
larroy
pushed a commit
to larroy/mxnet
that referenced
this pull request
Sep 28, 2019
* Numpy flip operator * Implement flip * fix some bug and add gpu test * register param and edit test * add testcase for backward * remove print * optimize 0-dim and 0-shape * adjust format and add doc in _symbol.py * fix bug in symbol * add flip in __all__ * fix format error * import ndarray * move flip implementation to np_matrix_op and remove test in gpu * delate redundant blank line * fix error in review * remove **kwargs and change copy * fix error in review * Fix import * Fix lint
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
operator flip